Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Update internal state of DatabaseChangeMonitor when external changes … #3503

Merged
merged 3 commits into from
Dec 13, 2017

Conversation

lenhard
Copy link
Member

@lenhard lenhard commented Dec 8, 2017

…are resolved

Attempts to fix #3498

It seems that the internal state of DatabaseChangeMonitor was not updated correctly when external changes were marked as resolved, causing the monitor to misbehave when you tried to save afterwards. I am not sure how this bug came into being and what caused the update to get lost. Adding an additional update step seems to resolve the problem, but I would really appreciate some more testing of this branch by other people.


  • Change in CHANGELOG.md described
  • Tests created for changes
  • Screenshots added (for bigger UI changes)
  • Manually tested changed features in running JabRef
  • Check documentation status (Issue created for outdated help page at help.jabref.org?)
  • If you changed the localization: Did you run gradle localizationUpdate?

@lenhard lenhard added the status: ready-for-review Pull Requests that are ready to be reviewed by the maintainers label Dec 8, 2017
Copy link
Member

@tobiasdiez tobiasdiez left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the fix! It seems to work fine on my side and the code looks also good to me (one small suggestion through).

@@ -1984,7 +1984,7 @@ public void resetChangeMonitor() {
}

public void updateTimeStamp() {
changeMonitor.ifPresent(DatabaseChangeMonitor::markAsSaved);
changeMonitor.ifPresent(DatabaseChangeMonitor::updateTimestampAndFileSize);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I would actually prefer to rename updateTimeStamp() to markAsSaved. That the DatabaseChangeMonitor uses the timestamp and file size to keep track of the last saved state, should be an implementation detail that is hidden to the outside world.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Alright, I changed the name back to what it was before.

@lenhard
Copy link
Member Author

lenhard commented Dec 13, 2017

It seems we're having a bit of a shortage of reviewers at the moment ;)

Since this is such a small one-line change that actually fixes a bug, I'll merge this now even if there's only one review.

@lenhard lenhard merged commit 21c7ba7 into master Dec 13, 2017
@tobiasdiez tobiasdiez deleted the fix-external-update branch December 13, 2017 14:23
Siedlerchr added a commit that referenced this pull request Dec 13, 2017
* upstream/master: (108 commits)
  Fetcher for IACR eprints (#3473)
  Update internal state of DatabaseChangeMonitor when external changes … (#3503)
  Fixes #3505: Another try to fix the NPE in the search bar (#3512)
  Replace ' with ' so that our HTML preview can handle it correctly
  Added a "Clear text" button in right click menu within the text boxes. (#3475)
  Add reset to English language after a test
  New translations JabRef_en.properties (German)
  Remove ampersand in non-menu localizations
  New translations JabRef_en.properties (German)
  New translations Menu_en.properties (German)
  New translations Menu_en.properties (German)
  New translations JabRef_en.properties (Vietnamese)
  New translations JabRef_en.properties (Italian)
  New translations Menu_en.properties (Italian)
  New translations JabRef_en.properties (Indonesian)
  New translations Menu_en.properties (Indonesian)
  New translations JabRef_en.properties (Greek)
  New translations Menu_en.properties (Greek)
  New translations Menu_en.properties (Japanese)
  New translations JabRef_en.properties (German)
  ...

# Conflicts:
#	build.gradle
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
status: ready-for-review Pull Requests that are ready to be reviewed by the maintainers
Projects
None yet
Development

Successfully merging this pull request may close these issues.

external changes to bib file make JabRef go mad
2 participants